-
Notifications
You must be signed in to change notification settings - Fork 14k
Offload intrinsic #147936
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Offload intrinsic #147936
Conversation
9118683 to
23722aa
Compare
This comment has been minimized.
This comment has been minimized.
| } | ||
|
|
||
| pub fn from_ty<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> Self { | ||
| OffloadMetadata { payload_size: get_payload_size(tcx, ty), mode: TransferKind::Both } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you already have the code here, I would add a small check for & or byVal (implies Mode ToGPU), vs &mut (implies Both).
In the future we would hope to analyze the & or byval case more, if we never read from it (before writing) then we could use a new mode 4, which allocates directly on the gpu.
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #148507) made this pull request unmergeable. Please resolve the merge conflicts. |
e0fd7be to
97a8e96
Compare
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #148721) made this pull request unmergeable. Please resolve the merge conflicts. |
3540edb to
e9d89ce
Compare
This comment has been minimized.
This comment has been minimized.
e9d89ce to
a08949b
Compare
This comment has been minimized.
This comment has been minimized.
a08949b to
9397d31
Compare
This comment has been minimized.
This comment has been minimized.
9397d31 to
7666b58
Compare
This comment has been minimized.
This comment has been minimized.
e38248d to
b34be91
Compare
| #[rustc_intrinsic] | ||
| pub const fn autodiff<F, G, T: crate::marker::Tuple, R>(f: F, df: G, args: T) -> R; | ||
|
|
||
| /// Generates the LLVM body of a wrapper function to offload a kernel `f`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have other backends besides LLVM, so intrinsics typically should be described in terms of what they do, not implementation details. Is that possible here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this intrinsic only makes sense in LLVM right now because it relies directly on LLVM's offload feature. that's why i wanted to specify the backend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there's a better way to proceed, please let me know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, seems like we did something similar for the autodiff intrinsic. I would assume that the concept of offloading is independent of LLVM, but maybe we don't have to figure out that full story at this point.
Are there docs for the LLVM offload feature you could link to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd say https://clang.llvm.org/docs/OffloadingDesign.html contains all the relevant details
ping @ZuseZ4 in case he has something better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, your link is probably the best overview. Offload grew out of OpenMP, which is also supported by other compilers like GCC. LLVM just put in some effort to split Offloading and OpenMP, so that the former is easier to use independently. https://gcc.gnu.org/projects/gomp/
ping @antoyo just for awareness.
With respect to a high-level explanation of this intrinsic:
We use a single-source, two-pass compilation approach. We compile all functions that should be offloaded for the device (e.g nvptx64, amdgcn-amd-amdhsa, intel in the future) and which are marked by our intrinsic. We then compile the code for the host (e.g. x86-64), where most of the offloading logic happens. On the host side, we generate calls to the openmp offload runtime, to inform it about the layout of the types (a simplified version of the autodiff TypeTrees). We also use the type system to figure out whether kernel arguments have to be moved only to the device (e.g. &[f32;1024]), from the device, or both (e.g. &mut [f64]). We then launched the kernel, after which we inform the runtime to end this environment and move data back (as far as needed).
There are obviously a lot of features and optimizations which we want to add in the future. The Rust frontend currently also mostly uses the OpenMP API, since it was more stable back when I started working on it. We intend to move over to the newer offload API, which is slightly lower level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use a single-source, two-pass compilation approach. We compile all functions that should be offloaded for the device (e.g nvptx64, amdgcn-amd-amdhsa, intel in the future) and which are marked by our intrinsic. We then compile the code for the host (e.g. x86-64), where most of the offloading logic happens. On the host side, we generate calls to the openmp offload runtime, to inform it about the layout of the types (a simplified version of the autodiff TypeTrees). We also use the type system to figure out whether kernel arguments have to be moved only to the device (e.g. &[f32;1024]), from the device, or both (e.g. &mut [f64]). We then launched the kernel, after which we inform the runtime to end this environment and move data back (as far as needed).
That also seems worth documenting somewhere, though I am not entirely sure where. And the type system interactions here definitely need to be discussed with t-opsem and probably other teams before anything in this area gets stabilized.
|
I'll do another round later, but can you also update https://rustc-dev-guide.rust-lang.org/offload/usage.html#usage, |
|
☔ The latest upstream changes (presumably #149013) made this pull request unmergeable. Please resolve the merge conflicts. |
b34be91 to
0cee58b
Compare
|
The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes. |
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I wanted to submit these yesterday.
| // Step 2) | ||
| let s_ident_t = generate_at_one(&cx); | ||
| let o = memtransfer_types[0]; | ||
| let o = memtransfer_types; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just use memtransfer_types directly
| metadata: &[OffloadMetadata], | ||
| types: &[&Type], | ||
| symbol: &str, | ||
| ) -> (&'ll llvm::Value, &'ll llvm::Value, &'ll llvm::Value, &'ll llvm::Value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You seem to pass those 4 around together, and it's a bit hard to tell from the function signature what you're returning. Can you create a small struct so we can return that instead, and add a one-sentence documentation to it? It looks like we need these four per kernel that we want to handle.
| let mut builder = SBuilder::build(cx, kernel_call_bb); | ||
|
|
||
| let types = cx.func_params_types(cx.get_type_of_global(called)); | ||
| let mut builder = SBuilder::build(cx, bb); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a FIXME, so we can get rid of this? I don't think it should be a permanent solution. I'm also somewhat confused about why they are unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i had a similar issue with autodiff, i think that, as the intrinsic is lowered relatively early in the compilation pipeline, it goes through more LLVM opt passes, and since there isn't yet any info that they will be used by the offloading feature, LLVM internalizes them (i tried to prevent them from being optimized by changing the linkage, but they always appeared as internal) and then removes them because unused internal variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i understand that in the first version of codegen, when this is done in fat LTO, LLVM is already aware of what will actually happen and doesn't modify them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We launch all LLVM passes at once via an LLVM PassManager, so it shouldn't change. But as long as it works, we can postpone investigations till later, the PR is enough of an improvement.
| &target_symbol, | ||
| ); | ||
|
|
||
| let bb = unsafe { llvm::LLVMGetInsertBlock(bx.llbuilder) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like you get a bb from a builder, just to then create a builder out of the bb inside of gen_call_handling, right? Can you directly pass the builder?
| // Step 0) | ||
| // %struct.__tgt_bin_desc = type { i32, ptr, ptr, ptr } | ||
| // %6 = alloca %struct.__tgt_bin_desc, align 8 | ||
| unsafe { llvm::LLVMRustPositionBuilderPastAllocas(builder.llbuilder, main_fn) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that you create / reuse a builder, are you sure that the tgt_bin_desc would get an alloca in the right position in the first bb (and it nost just working in the test by coincidence)?
E.g.
fn main() {
if (condition) {
} else {
}
core::intrinsic::offload(args);
}If the builder is on the intrinsic, the alloca wouldn't land where it should (in the beginning).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the idea (or at least how i'd imagined it) is that when expanding the future macro, the wrapper function should always contain only the intrinsic, so we can generate all the logic sequentially
if you mean that it needs to be at the beginning of the first bb of the program, just let me know and i'll change that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, allocas should all be together at the beginning, so moving the builder via LLVMRustPositionBuilderPastAllocas (and putting the builder back into the old place) would be the way to go.
It might work if you put them elsewhere, but LLVM opt passes don't really expect that, so we're likely to miss out on some optimizations.
I agree that we should later distinguish better between the kernel launch intrinsic and the globals that are somewhat independent of the number of kernel launches.
| let a5 = builder.direct_alloca(tgt_kernel_decl, Align::EIGHT, "kernel_args"); | ||
|
|
||
| // Step 1) | ||
| unsafe { llvm::LLVMRustPositionBefore(builder.llbuilder, kernel_call) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question for the position of the memset without repositioning the builder.
| ); | ||
| } | ||
|
|
||
| fn codegen_offload<'ll, 'tcx>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some docs to this function?
|
☔ The latest upstream changes (presumably #148151) made this pull request unmergeable. Please resolve the merge conflicts. |
This PR implements the minimal mechanisms required to run a small subset of arbitrary offload kernels without relying on hardcoded names or metadata.
offload(kernel, (..args)): an intrinsic that generates the necessary host-side LLVM-IR code.rustc_offload_kernel: a builtin attribute that marks device kernels to be handled appropriately.Example usage (pseudocode):